update(heatmap-basic): seaborn — comprehensive quality review#4253
update(heatmap-basic): seaborn — comprehensive quality review#4253github-actions[bot] merged 5 commits intomainfrom
Conversation
Comprehensive quality review: fix weaknesses from prior reviews, preserve strengths, improve quality across all dimensions.
AI Review - Attempt 1/3Image Description
Quality Score: 82/100Criteria ChecklistVisual Quality (24/30)
Design Excellence (12/20)
Spec Compliance (14/15)
Data Quality (14/15)
Code Quality (9/10)
Library Mastery (9/10)
Strengths
Weaknesses
Verdict: REJECTED |
There was a problem hiding this comment.
Pull request overview
This PR updates the seaborn implementation of the heatmap-basic specification to address weaknesses from prior quality reviews. The changes include updating library versions, modifying the specification to add a "matrix" data type tag and revise ordering guidelines, and significantly refactoring the implementation to use hierarchical clustering.
Changes:
- Updated specification.yaml with new timestamp and added "matrix" data type tag
- Modified specification.md to change clustering guidance to logical ordering guidance
- Switched implementation from
sns.heatmap()tosns.clustermap()with row clustering enabled - Updated metadata to Python 3.14.3, claude-opus-4-6, with quality score pending review
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plots/heatmap-basic/specification.yaml | Added "matrix" data type tag and updated timestamp |
| plots/heatmap-basic/specification.md | Changed guidance from clustering to logical ordering |
| plots/heatmap-basic/metadata/seaborn.yaml | Updated versions (Python 3.14.3, claude-opus-4-6) and nullified quality score |
| plots/heatmap-basic/implementations/seaborn.py | Replaced sns.heatmap() with sns.clustermap(), added hierarchical clustering |
| @@ -1,7 +1,7 @@ | |||
| """ pyplots.ai | |||
There was a problem hiding this comment.
The header docstring format is inconsistent with the codebase convention. Most implementations use a space after the opening triple quotes. This should be """ pyplots.ai not """pyplots.ai.
| # Plot - clustermap groups similar departments via hierarchical clustering | ||
| cmap = sns.diverging_palette(240, 10, as_cmap=True) | ||
| g = sns.clustermap( | ||
| data, | ||
| annot=True, | ||
| fmt=".0f", | ||
| cmap="RdBu", | ||
| cmap=cmap, | ||
| center=50, | ||
| xticklabels=months, | ||
| yticklabels=departments, | ||
| linewidths=1, | ||
| linecolor="white", | ||
| cbar_kws={"label": "Performance Score", "shrink": 0.75}, | ||
| annot_kws={"fontsize": 16}, | ||
| ax=ax, | ||
| annot_kws={"fontsize": 14}, | ||
| figsize=(16, 10), | ||
| row_cluster=True, | ||
| col_cluster=False, | ||
| dendrogram_ratio=0.08, | ||
| cbar_pos=None, | ||
| vmin=0, | ||
| vmax=100, | ||
| ) |
There was a problem hiding this comment.
Using sns.clustermap() with hierarchical clustering is inappropriate for a "basic" heatmap specification. The specification for heatmap-basic asks for a simple matrix heatmap with logical ordering (alphabetical, by magnitude, or by similarity), not hierarchical clustering with dendrograms. Clustering functionality is the defining feature of the separate heatmap-clustered specification. This implementation should use sns.heatmap() instead, similar to the matplotlib implementation and the heatmap-correlation seaborn implementation. The current approach violates the principle that each specification should demonstrate a distinct plot type.
| # Colorbar with proper placement | ||
| sm = plt.cm.ScalarMappable(cmap=cmap, norm=plt.Normalize(0, 100)) | ||
| cbar = g.fig.colorbar(sm, ax=g.ax_heatmap, location="right", shrink=0.7, pad=0.12) | ||
| cbar.set_label("Performance Score", fontsize=18) | ||
| cbar.ax.tick_params(labelsize=14) |
There was a problem hiding this comment.
The manual colorbar creation is unnecessarily complex when using clustermap. However, if switching to sns.heatmap() as recommended, this entire colorbar section would be handled automatically by the heatmap function's cbar_kws parameter, similar to the pattern used in heatmap-correlation seaborn implementation.
| # Style | ||
| g.ax_heatmap.set_xlabel("Month", fontsize=20) | ||
| g.ax_heatmap.set_ylabel("") | ||
| g.ax_heatmap.tick_params(axis="x", labelsize=16) | ||
| g.ax_heatmap.tick_params(axis="y", labelsize=16, rotation=0) | ||
| g.fig.suptitle("heatmap-basic · seaborn · pyplots.ai", fontsize=24, y=1.02) |
There was a problem hiding this comment.
The styling code references clustermap-specific attributes (g.ax_heatmap, g.fig.suptitle). When switching to sns.heatmap() as recommended, this should be simplified to use the standard matplotlib ax.set_xlabel(), ax.set_ylabel(), ax.set_title() pattern seen in the heatmap-correlation implementation.
Attempt 1/3 - fixes based on AI review
🔧 Repair Attempt 1/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
AI Review - Attempt 2/3Image Description
Quality Score: 92/100Criteria ChecklistVisual Quality (28/30)
Design Excellence (15/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (10/10)
Library Mastery (10/10)
Strengths
Weaknesses
Verdict: APPROVED |
Summary
Updated seaborn implementation for heatmap-basic.
Changes: Comprehensive quality review — fix weaknesses from prior reviews, preserve strengths, improve quality across all dimensions.
Changes
Test Plan
Generated with Claude Code
/updatecommand